Rework next streetid computation #370

Grufoony · 2025-11-21T13:16:47Z

No description provided.

codecov · 2025-11-21T13:19:59Z

Codecov Report

❌ Patch coverage is 82.71605% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 83.34%. Comparing base (2fac69f) to head (b8a9740).
⚠️ Report is 6 commits behind head on main.

Files with missing lines	Patch %	Lines
src/dsf/mobility/RoadDynamics.hpp	78.46%	14 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #370      +/-   ##
==========================================
- Coverage   83.56%   83.34%   -0.23%     
==========================================
  Files          51       52       +1     
  Lines        5216     5207       -9     
  Branches      602      593       -9     
==========================================
- Hits         4359     4340      -19     
- Misses        845      855      +10     
  Partials       12       12

Flag	Coverage Δ
unittests	`83.34% <82.71%> (-0.23%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

src/dsf/mobility/RoadDynamics.hpp

-    std::optional<Id> m_nextStreetId(std::unique_ptr<Agent> const& pAgent,
-                                     Id NodeId,
-                                     std::optional<Id> streetId = std::nullopt);
+    std::optional<Id> m_nextStreetId(const std::unique_ptr<Agent>& pAgent,


src/dsf/mobility/RoadDynamics.hpp

                           static_cast<double>(this->time_step() - pAgent->spawnTime())});
    --m_nAgents;
-    return std::move(pAgent);
+    return pAgent;


src/dsf/mobility/RoadDynamics.hpp

+    // Get current street information
+    std::optional<Id> previousNodeId = std::nullopt;
+    std::set<Id> forbiddenTurns;
+    double speedCurrent = 1.0;


src/dsf/mobility/RoadDynamics.hpp

+    std::optional<Id> previousNodeId = std::nullopt;
+    std::set<Id> forbiddenTurns;
+    double speedCurrent = 1.0;
+    if (pAgent->streetId().has_value()) {


src/dsf/mobility/RoadDynamics.hpp

+        pathTargets = it->second->path().at(pNode->id());
+      } catch (const std::out_of_range&) {
+        throw std::runtime_error(std::format("No path found for itinerary {} at node {}",
+                                             pAgent->itineraryId(),


src/dsf/mobility/RoadDynamics.hpp

-            allowedTargets.insert(pathTargets.begin(), pathTargets.end());
-            hasItineraryConstraints = true;
+    for (const auto outEdgeId : outgoingEdges) {
+      if (forbiddenTurns.contains(outEdgeId)) {


src/dsf/mobility/RoadDynamics.hpp


-    if (possibleEdgeIds.empty()) {
+    // Select street based on weighted probabilities
+    if (transitionProbabilities.empty()) {


Copilot

Pull request overview

This PR refactors the m_nextStreetId computation to consolidate the street selection logic into a unified probability-based approach. The changes eliminate separate code paths for random vs. non-random agents, moving several simple getter methods from .cpp files to inline implementations in headers for potential performance improvements.

Unified street selection algorithm using weighted probabilities for all agent types
Moved simple accessor methods to inline header implementations with noexcept and auto return types
Updated function signatures to pass node objects directly instead of node IDs

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
src/dsf/mobility/Street.hpp	Added `noexcept` to `isStochastic()` virtual methods and attempted to add `constexpr` (incorrectly)
src/dsf/mobility/RoadJunction.hpp	Converted virtual method declarations to inline implementations with attempted `constexpr` (incorrectly)
src/dsf/mobility/RoadJunction.cpp	Removed implementations now defined inline in header
src/dsf/mobility/RoadDynamics.hpp	Complete rework of `m_nextStreetId` to use unified probability-based selection; updated function signature; removed redundant `std::move` calls
src/dsf/mobility/Road.hpp	Converted getter methods to inline implementations using `auto` return type
src/dsf/mobility/Road.cpp	Removed implementations now defined inline in header
src/dsf/mobility/Agent.hpp	Attempted to add `constexpr` to `isRandom()` method (incorrectly)

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

src/dsf/mobility/RoadDynamics.hpp

Copilot · 2025-11-21T13:58:07Z

src/dsf/mobility/RoadDynamics.hpp

    if (outgoingEdges.size() == 1) {
+      auto const& pStreetOut{this->graph().edge(outgoingEdges[0])};
+      if (!pNode->isRoundabout() && pStreetOut->target() == pStreetOut->source()) {
+        return std::nullopt;
+      }
      return outgoingEdges[0];
    }


The early return for single outgoing edge bypasses important validation checks. It doesn't verify:

Whether the edge is in forbiddenTurns for the current street

Whether the edge is a valid path target for non-random agents with itineraries

This could lead to agents taking forbidden or invalid paths. Consider moving this check after gathering forbidden turns and path targets, or at minimum validating these conditions before the early return.

src/dsf/mobility/Agent.hpp

src/dsf/mobility/Street.hpp

src/dsf/mobility/RoadJunction.hpp

Copilot · 2025-11-21T13:58:08Z

src/dsf/mobility/RoadDynamics.hpp

-      if (forbiddenTargetNodes.count(targetNode)) {
-        continue;
+      // Apply error probability for non-random agents
+      if (this->m_errorProbability.has_value() && pathTargets.empty()) {


The condition on line 563 appears incorrect. It should check !pathTargets.empty() instead of pathTargets.empty() to apply error probability only when path targets exist (i.e., for non-random agents). The current logic applies error probability when pathTargets is empty, which doesn't match the intended behavior.

Suggested change

if (this->m_errorProbability.has_value() && pathTargets.empty()) {

if (this->m_errorProbability.has_value() && !pathTargets.empty()) {

src/dsf/mobility/Road.hpp

src/dsf/mobility/RoadDynamics.hpp

+      } catch (const std::out_of_range&) {
+        if (!m_itineraries.contains(pAgent->itineraryId())) {
+          throw std::runtime_error(
+              std::format("No itinerary found with id {}", pAgent->itineraryId()));


src/dsf/mobility/RoadDynamics.hpp

-    // For non-random agents, get allowed targets from itinerary
-    std::unordered_set<Id> allowedTargets;
-    bool hasItineraryConstraints = false;
+    for (const auto outEdgeId : outgoingEdges) {


src/dsf/mobility/RoadDynamics.hpp

-    if (possibleEdgeIds.size() == 1) {
-      return possibleEdgeIds[0];
+    if (transitionProbabilities.size() == 1) {
+      return transitionProbabilities.cbegin()->first;


src/dsf/mobility/RoadDynamics.hpp

+      }
+    }
+    // Return last one as fallback
+    return std::prev(transitionProbabilities.cend())->first;


src/dsf/mobility/RoadDynamics.hpp

  template <typename delay_t>
    requires(is_numeric_v<delay_t>)
  void RoadDynamics<delay_t>::m_evolveAgents() {
+    if (m_agents.empty()) {


Grufoony added 3 commits November 21, 2025 09:53

Enhance inline and constexpr qualifiers

baf08bb

Refactor params of m_nextStreetId function

cc93be5

Simplify m_nextStreetId function

367fb69

github-advanced-security bot found potential problems Nov 21, 2025

View reviewed changes

Grufoony requested a review from Copilot November 21, 2025 13:54

Copilot started reviewing on behalf of Grufoony November 21, 2025 13:55 View session

Copilot finished reviewing on behalf of Grufoony November 21, 2025 13:57

Copilot AI reviewed Nov 21, 2025

View reviewed changes

Grufoony and others added 2 commits November 21, 2025 15:05

Bump version

509cd01

Copilot fixes

b8a9740

github-advanced-security bot found potential problems Nov 22, 2025

View reviewed changes

Grufoony merged commit 7afeff6 into main Nov 22, 2025
39 of 41 checks passed

Grufoony deleted the reworkNextStreetId branch November 22, 2025 18:41

	if (this->m_errorProbability.has_value() && pathTargets.empty()) {
	if (this->m_errorProbability.has_value() && !pathTargets.empty()) {

Rework next streetid computation #370

Rework next streetid computation #370

Uh oh!

Conversation

Grufoony commented Nov 21, 2025

Uh oh!

codecov bot commented Nov 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Check notice

Check notice

Uh oh!

Check notice

Check notice

Uh oh!

Check notice

Check notice

Check notice

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Nov 21, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Check notice

Check notice

Check notice

Check notice

Check notice

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov bot commented Nov 21, 2025 •

edited

Loading